Skip to content

Add Coach-inspired session quality scoring and insights - #645

Merged
wesm merged 3 commits into
mainfrom
quality/phase-6-validation-rollout
Jun 19, 2026
Merged

Add Coach-inspired session quality scoring and insights#645
wesm merged 3 commits into
mainfrom
quality/phase-6-validation-rollout

Conversation

@mariusvniekerk

@mariusvniekerk mariusvniekerk commented Jun 11, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

This PR adds session-quality measurement to agentsview. It turns archived sessions into inspectable quality signals: deterministic scoring for behavior that can be measured locally, plus opt-in generated recommendations that summarize those facts without becoming canonical score data.

The core goal is explainable review. Quality labels should be backed by stored signal counts, filtered evidence, score explanations, and session drilldowns so reviewers can decide whether the measurements are useful before treating them as calibrated defaults.

Source Inspiration

This work was shaped by Microsoft's AI Engineering Coach, especially its rule catalog, scoring detector, insights analyzer, and metric definitions.

The agentsview implementation adapts those ideas to a local, multi-agent session archive. Coach-inspired rules such as lazy prompting, missing file context, repeated prompts, missing spec structure, runaway loops, prompt maturity, premium-model waste, and sustainable-work-pattern signals were reviewed and then sorted into deterministic scoring, aggregate analytics, generated-insight inputs, already-covered behavior, or blocked future work depending on the data available in agentsview.

That distinction is intentional: this PR borrows the framing and rule inventory, but it does not make Copilot/VS Code-specific assumptions or use an LLM as the source of truth for score computation.

What This Adds

  • A documented mapping from Coach rules to agentsview signal families, including what is scoreable now, what belongs in analytics or generated insights, and what is blocked by missing metadata.
  • Deterministic prompt/context/workflow heuristics for local session scoring, including short task starts, missing criteria, missing verification, repeated prompts, missing code context, and repeated failing tool cycles.
  • Persisted quality signal counts across SQLite, PostgreSQL, DuckDB, service, sync, and generated frontend API surfaces without recreating the archive database.
  • Analytics and Insights surfaces for quality pattern summaries, grade/calibration metadata, signal totals, filtered evidence drilldowns, and deep links back to affected sessions.
  • An opt-in canned LLM insight path using fixed templates, deterministic aggregate payloads, validation, provenance, cache keys, and template versions. Generated prose stays separate from health scores and signal rows.
  • Rollout documentation, golden score assertions, resync preservation checks, frontend coverage, and screenshots so reviewers can evaluate the model before final calibration.

Important Boundaries

  • LLMs do not perform canonical scoring. Session health remains deterministic and locally computable.
  • Generated recommendations are opt-in, cached, provenance-bearing, and kept out of score/signal writes.
  • Coach rules that require missing metadata, such as agent mode, plan mode, slash commands, approval mode, AI-generated LOC, review timing, or request cancellation, are documented but not scored.
  • Timing and work-pattern ideas such as weekend or late-night activity remain analytics material, not session-quality penalties.
  • Archive migrations remain additive. Existing session, orphaned, trashed, and saved insight data must be preserved.

Screenshots

Quality patterns

Evidence drilldown

Generated insight archive

Review Focus

  • Whether the score-affecting heuristics are conservative enough and have acceptable false-positive behavior.
  • Whether the scoring caps, grade deltas, and explanation fields are understandable enough for calibration.
  • Whether the signal persistence/API contract is complete across SQLite, PostgreSQL, DuckDB, service, sync, and frontend clients.
  • Whether the Insights page makes deterministic evidence easy to inspect before reading generated recommendation text.
  • Whether generated insights are clearly bounded as optional summaries rather than a second source of scoring truth.
  • Whether the rollout checklist is sufficient before enabling these semantics broadly on real archives.

Remaining Rollout Work

Before treating this as final calibration, run a representative local archive pass and record score deltas, threshold crossings, false-positive themes, runtime impact, and source diversity. The added rollout document captures the expected report shape and the contract checklist for future signal/API changes.

@roborev-ci

roborev-ci Bot commented Jun 11, 2026

Copy link
Copy Markdown

roborev: Combined Review (1c68fc1)

Medium confidence: one medium issue needs attention before merge.

Medium

  • internal/server/insights.go:719 — Canned insight generation rebuilds aggregates with only date/project/automated scope and hard-coded UTC, ignoring the active analytics filters used by the visible dashboard such as session agent, machine, termination, min-user-message, one-shot inclusion, and browser timezone. This can generate and cache recommendations for a broader or different cohort than the user is viewing.

    Fix: Add a separate session-filter payload to generation requests, send the current analytics filter params, apply them to AnalyticsFilter, UsageFilter, and SessionFilter, and include them in provenance/cache keys.

Panel: ci_default_security | Synthesis: codex, 7s | Members: codex_default (codex/default, done, 17m30s), codex_security (codex/security, done, 4m51s) | Total: 22m28s

@cpcloud

cpcloud commented Jun 12, 2026

Copy link
Copy Markdown
Contributor

I think my only feedback right now is that I would probably like to see the all the recommendations first and then be presented with the detailed data.

@mariusvniekerk
mariusvniekerk force-pushed the quality/phase-6-validation-rollout branch from 1c68fc1 to b10b1d1 Compare June 18, 2026 15:14
@roborev-ci

roborev-ci Bot commented Jun 18, 2026

Copy link
Copy Markdown

roborev: Combined Review (b10b1d1)

Medium issue found; no Critical or High findings.

Medium

  • frontend/src/lib/stores/usage.svelte.ts:226 — The shared termination session filter is not forwarded to usage summary/top-session requests, so the Usage page can show costs for sessions outside the selected termination scope even though the backend and generated client support the filter. Add termination: sessionFilters.termination || undefined to baseParams() and include termination in the Usage page URL/session filter key handling.

Panel: ci_default_security | Synthesis: codex, 6s | Members: codex_default (codex/default, done, 15m29s), codex_security (codex/security, done, 5m40s) | Total: 21m15s

@roborev-ci

roborev-ci Bot commented Jun 18, 2026

Copy link
Copy Markdown

roborev: Combined Review (f7b4297)

Medium findings remain; no Critical or High issues reported.

Medium

  • internal/postgres/analytics.go:133 - PostgreSQL analytics treats AnalyticsFilter.Machine as one literal value, while SQLite and the UI support comma-separated machine filters. In PG serve, machine=a,b matches a literal machine name instead of either selected machine. Split and trim f.Machine and build an IN (...) predicate with placeholders to match SQLite behavior.

  • internal/duckdb/analytics_usage.go:175 - DuckDB analytics ignores AutomatedScope and still relies on ExcludeAutomated. The frontend sends automated_scope=all|automated without include_automated, so DuckDB serve can keep filtering to human sessions when the user selects “Both” or “Only automated”. Normalize f.AutomatedScope like SQLite/PostgreSQL and drive automated-session predicates from that scope.

Panel: ci_default_security | Synthesis: codex, 8s | Members: codex_default (codex/default, done, 8m32s), codex_security (codex/security, done, 4m49s) | Total: 13m29s

@roborev-ci

roborev-ci Bot commented Jun 18, 2026

Copy link
Copy Markdown

roborev: Combined Review (c64db84)

Medium issues remain in DuckDB analytics parity and timestamp handling.

Medium

  • internal/duckdb/analytics_usage.go:1866
    duckSignalMessages coalesces a DuckDB TIMESTAMP column with '' and scans it into a string, unlike the rest of the DuckDB store which scans timestamp values as any and normalizes with formatDBTime. Signal evidence can fail or return non-RFC3339 timestamps for rows with null timestamps.
    Fix: Select timestamp directly, scan into any, and set m.Timestamp = formatDBTime(ts).

  • internal/duckdb/analytics_usage.go:2122
    DuckDB usage queries ignore UsageFilter.Termination, while SQLite/Postgres now apply it and the frontend sends it. In duckdb serve, usage summaries, comparisons, and top-cost sessions will not respect active/stale/unclean/clean filters.
    Fix: Add DuckDB usage termination predicates in appendDuckUsageSessionFilterClauses, matching the SQLite/Postgres semantics.

Panel: ci_default_security | Synthesis: codex, 7s | Members: codex_default (codex/default, done, 12m49s), codex_security (codex/security, done, 5m24s) | Total: 18m20s

@mariusvniekerk mariusvniekerk changed the title RFC: Phase 6 validation rollout Add session quality signals and insights rollout Jun 18, 2026
@mariusvniekerk mariusvniekerk changed the title Add session quality signals and insights rollout Add session quality signals and generated insights Jun 18, 2026
@mariusvniekerk mariusvniekerk changed the title Add session quality signals and generated insights Add Coach-inspired session quality scoring and insights Jun 18, 2026
@mariusvniekerk
mariusvniekerk marked this pull request as ready for review June 18, 2026 21:18
@roborev-ci

roborev-ci Bot commented Jun 18, 2026

Copy link
Copy Markdown

roborev: Combined Review (a21c0cd)

Medium-risk issue found; no Critical or High findings.

Medium

  • internal/server/insights.go:529 - The Coach portion of canned insight payloads is scoped with db.SessionFilter.DateFrom/DateTo, which ignores filters.Timezone. Signals and usage in the same payload use timezone-aware filters, so non-UTC users can receive generated insight prompts where the Coach summary covers a different session set than the signal and usage aggregates.

    Fix: Apply the same local-date semantics before building the Coach summary, either by using a timezone-aware store query or by overfetching and filtering session activity dates in filters.Timezone.

Panel: ci_default_security | Synthesis: codex, 6s | Members: codex_default (codex/default, done, 14m21s), codex_security (codex/security, done, 6m9s) | Total: 20m36s

@roborev-ci

roborev-ci Bot commented Jun 18, 2026

Copy link
Copy Markdown

roborev: Combined Review (ffc4df9)

Review verdict: one medium issue should be fixed before merge; no high or critical findings reported.

Medium

  • internal/server/insights.go:60 - Canned insight filter timezones are trimmed/defaulted but not validated. Invalid timezones can flow into analytics, usage, and Coach session aggregation paths with inconsistent fallback behavior instead of matching the 400 response used by other analytics APIs.
    • Fix: validate filters.Timezone with time.LoadLocation after defaulting and reject invalid values before building the canned insight payload.

Panel: ci_default_security | Synthesis: codex, 10s | Members: codex_default (codex/default, done, 13m14s), codex_security (codex/security, done, 6m38s) | Total: 20m2s

@roborev-ci

roborev-ci Bot commented Jun 18, 2026

Copy link
Copy Markdown

roborev: Combined Review (cc37881)

Medium issue found; no Critical or High findings.

Medium

  • frontend/src/lib/components/insights/qualityPatterns.ts:367
    The tool reliability pattern bases severity, affected session count, and retry/edit-churn driver session counts on sessions_with_failures. Backend aggregation only increments that field for ToolFailureSignalCount > 0, while retry and edit-churn signals are computed independently. Sessions with only retries or edit churn can therefore be hidden as 0 sessions / Clear, and retry/churn rows can get the wrong session count.

    Fix: Track sessions with retries and edit churn separately in the tool health aggregate, or derive driver counts from signals.calibration["tool_retries"].affected_sessions and signals.calibration["edit_churn"].affected_sessions.

Panel: ci_default_security | Synthesis: codex, 6s | Members: codex_default (codex/default, done, 11m41s), codex_security (codex/security, done, 4m5s) | Total: 15m52s

@roborev-ci

roborev-ci Bot commented Jun 19, 2026

Copy link
Copy Markdown

roborev: Combined Review (c00b82d)

Medium issue found; no high or critical findings.

Medium

  • internal/duckdb/analytics_usage.go:2140 - DuckDB usage queries ignore UsageFilter.AutomatedScope, so dashboard-scoped canned insight payloads can include the wrong session set compared with SQLite/Postgres. human still includes automated usage, automated does not restrict to automated sessions, and one-shot exclusion does not preserve automated one-shots for non-human scopes.
    • Fix: Mirror the SQLite/Postgres usage filter logic in appendDuckUsageSessionFilterClauses, including normalized automated scope, automated-scope predicates, and the automated one-shot exemption. Add DuckDB usage coverage for human, all, and automated scopes.

Panel: ci_default_security | Synthesis: codex, 6s | Members: codex_default (codex/default, done, 14m10s), codex_security (codex/security, done, 3m41s) | Total: 17m57s

@wesm

wesm commented Jun 19, 2026

Copy link
Copy Markdown
Member

rebasing

@wesm
wesm force-pushed the quality/phase-6-validation-rollout branch from c00b82d to 215791f Compare June 19, 2026 13:13
@roborev-ci

roborev-ci Bot commented Jun 19, 2026

Copy link
Copy Markdown

roborev: Combined Review (215791f)

Summary verdict: Changes need fixes before merge due to two medium behavioral regressions.

Medium

  • Location: frontend/src/lib/components/insights/InsightsPage.svelte:520
    Problem: The Insights page now drives its date controls and generation payload from analytics.from/to, but the Activity panel link still pre-fills insights.dateFrom/dateTo. Opening “Open in Insights page” loses the panel’s range, so generation runs for the analytics default/current range instead.
    Fix: Initialize/sync analytics.from/to from the prefilled insights dates when entering the Insights route, or update the Activity link to set the analytics date range before navigating.

  • Location: internal/server/huma_routes_insights.go:390
    Problem: activityRangeSummary always sets ExcludeAutomated: true, even after humaGenerateInsight normalizes and passes automated_scope to BuildPrompt. Requests for automated_scope=automated or all get a session list for that scope but an activity summary for human-only sessions, producing contradictory prompt context.
    Fix: Pass AutomatedScope: req.AutomatedScope into the analytics filter and remove the hard-coded human-only filter/comment.

Panel: ci_default_security | Synthesis: codex, 8s | Members: codex_default (codex/default, done, 22m59s), codex_security (codex/security, done, 7m1s) | Total: 30m8s

@wesm
wesm force-pushed the quality/phase-6-validation-rollout branch from 215791f to ffb1e65 Compare June 19, 2026 15:32
@roborev-ci

roborev-ci Bot commented Jun 19, 2026

Copy link
Copy Markdown

roborev: Combined Review (ffb1e65)

Medium issue found: DuckDB usage filtering is inconsistent with SQLite/PostgreSQL for automated scope handling.

Medium

  • internal/duckdb/analytics_usage.go:2143
    DuckDB usage filters still apply legacy ExcludeOneShot / ExcludeAutomated booleans directly and ignore UsageFilter.AutomatedScope. A query with AutomatedScope: "automated" can include human sessions, and ExcludeOneShot still drops one-shot automated sessions, unlike the SQLite and PostgreSQL paths.
    Fix: Mirror the SQLite/PG logic: normalize with duckNormalizeAutomatedScope, exempt automated sessions from the one-shot predicate when scope is not human, append duckAutomatedScopePredicate(scope, "COALESCE(s.is_automated, FALSE)"), and add a DuckDB usage-scope regression test.

Panel: ci_default_security | Synthesis: codex, 34s | Members: codex_default (codex/default, done, 16m51s), codex_security (codex/security, done, 4m38s) | Total: 22m3s

@wesm
wesm force-pushed the quality/phase-6-validation-rollout branch from ffb1e65 to 5baa768 Compare June 19, 2026 16:08
@roborev-ci

roborev-ci Bot commented Jun 19, 2026

Copy link
Copy Markdown

roborev: Combined Review (5baa768)

Code needs changes before merge: one medium backend parity issue remains.

Medium

  • internal/duckdb/analytics_usage.go:2143 - DuckDB usage filtering ignores UsageFilter.AutomatedScope. SQLite and PostgreSQL honor human/all/automated scopes and exempt automated sessions from the one-shot filter when scope is not human, but DuckDB still only checks ExcludeOneShot/ExcludeAutomated. This can make DuckDB usage totals, top-cost sessions, session counts, and canned insight usage payloads return a different session set for automated or all scopes.

    Fix: Mirror the SQLite/PostgreSQL logic in appendDuckUsageSessionFilterClauses: normalize f.AutomatedScope, apply the one-shot automated exemption for non-human scopes, and add duckAutomatedScopePredicate(scope, "COALESCE(s.is_automated, FALSE)").

Panel: ci_default_security | Synthesis: codex, 14s | Members: codex_default (codex/default, done, 14m18s), codex_security (codex/security, done, 3m21s) | Total: 17m53s

@roborev-ci

roborev-ci Bot commented Jun 19, 2026

Copy link
Copy Markdown

roborev: Combined Review (c3cb4b1)

No issues found.

Panel: ci_default_security | Synthesis: codex | Members: codex_default (codex/default, done, 10m38s), codex_security (codex/security, done, 7m3s) | Total: 17m41s

mariusvniekerk and others added 2 commits June 19, 2026 12:43
@mariusvniekerk @wesm
Document Coach heuristic signal map
80eaa94 
- Add deterministic session heuristic scorers
- fix: recognize search tool context in heuristics
- Persist quality heuristic signals
- Fix quality signal review gaps
- Add deterministic quality insights dashboard
- Fix quality insights review findings
- Surface all deterministic quality pattern signals
- Add opt-in canned LLM insights
- Fix canned insight review findings
- Normalize canned insight focus before caching
- Add Coach-derived inputs for canned insights
- Fix canned Coach insight cache stability
- Add quality rollout validation scaffolding
- Update quality rollout e2e after restack
- Fix quality rollout review findings
- Document quality validation gaps
- Add generated insight deep links
- Report canned insight validation details
- Add insight session scope controls
- Use icon actions for generated insight deletion
- Widen insights project filter typeahead
- Clarify generated insight generator choices
- Feed model breakdowns to cost insights
- Fix automated scope review findings
- Fix insights toolbar responsive height
- Compact insights project filter
- Stabilize insights scope dropdown width
- Clarify context setup pressure coverage
- Require failures for tool-cycle signal
- Add calibrated insight evidence drilldowns
- Unify date range selector state
- Narrow short-start quality signal
- Fix insight evidence review findings
- Add phase 6 RFC screenshots
- Scope canned insights to dashboard filters
- Restore generated analytics API parity
- Scope usage summaries to termination filters
- Align backend analytics filters
- Align DuckDB usage and signal evidence filters
- fix: align canned coach timezone filtering
- fix: validate canned insight timezones
- fix: count tool reliability driver sessions
- test: expect browser timezone in canned insight retry call
- fix: trim comma-separated agent filters in SQLite analytics
- fix: align insight automated scope and usage activity filters
- fix: align SQLite termination filter and insights automated scope
- fix: honor automated scope in DuckDB usage filters
@wesm
fix: honor top-level canned insight timezone
8cbccbc 
Canned insight requests still expose the top-level timezone field, and generated clients can send it without a nested filters object. Normalization previously defaulted canned filters to UTC before consulting that field, so cache keys and local-day aggregates diverged from the requested browser timezone.

Use the top-level timezone as the fallback whenever filters.timezone is absent, while preserving the nested filter timezone as the explicit override.
@wesm
wesm force-pushed the quality/phase-6-validation-rollout branch from c3cb4b1 to 8cbccbc Compare June 19, 2026 17:43
@roborev-ci

roborev-ci Bot commented Jun 19, 2026

Copy link
Copy Markdown

roborev: Combined Review (8cbccbc)

Summary verdict: one medium issue needs attention; no critical or high findings were reported.

Medium

  • frontend/src/lib/api/generated/models/DbSession.ts:5, frontend/src/lib/api/generated/models/ServiceSessionDetail.ts:5, frontend/src/lib/api/generated/models/DbSignalSessionsResponse.ts:6
    • Problem: The generated API types are out of sync with the new quality/evidence responses. Session models omit the runtime quality_signals object, and signal evidence sessions are typed as any[] instead of the generated DbSignalSessionExample[].
    • Fix: Expose these fields through schema-visible response DTOs or Huma schema overrides, then regenerate the client.

Panel: ci_default_security | Synthesis: codex, 6s | Members: codex_default (codex/default, done, 25m48s), codex_security (codex/security, done, 5m13s) | Total: 31m7s

@wesm
fix: expose quality signal API schemas
e38d1cd 
Runtime session responses already include grouped quality signal data, but the OpenAPI schema hid that field from generated clients. That left frontend callers without the typed quality_signals object even though the server returned it.

Signal evidence sessions also need to remain a non-null typed array in the schema so client generation keeps DbSignalSessionExample instead of collapsing the field to any[].
@roborev-ci

roborev-ci Bot commented Jun 19, 2026

Copy link
Copy Markdown

roborev: Combined Review (e38d1cd)

Clean overall: no Medium, High, or Critical findings reported.

The only findings were Low severity test-style/heuristic/UTF-8 truncation nits, so they are omitted per the review-combination rules.

Panel: ci_default_security | Synthesis: codex, 4s | Members: codex_default (claude-code/default, done, 10m38s), codex_security (claude-code/security, done, 3m49s) | Total: 14m31s

@wesm
wesm merged commit 1ba339b into main Jun 19, 2026
21 checks passed
@wesm
wesm deleted the quality/phase-6-validation-rollout branch June 19, 2026 20:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cpcloud cpcloud cpcloud approved these changes

Assignees

No one assigned

Labels

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@mariusvniekerk @cpcloud @wesm